fix: tool api call broken when user answer no to when asked for confirmation #371
+94
−23
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jrmi
changed the title
jrmi
commented
Dec 30, 2024
jrmi
commented
Dec 30, 2024
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to 2d25236 in 1 minute and 12 seconds
More details
- Looked at
223lines of code in7files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. gptme/llm/llm_anthropic.py:203
- Draft comment:
The change frommessage["role"] == "system"tomessage["role"] == "user"seems incorrect. The original condition was likely correct as it was handling system messages withcall_id. Please verify the intent of this change. - Reason this comment was not posted:
Comment did not seem useful.
2. gptme/llm/llm_openai.py:287
- Draft comment:
The function_merge_tool_results_with_same_call_idis defined but not used in this file. Consider integrating it where necessary or removing it if not needed. - Reason this comment was not posted:
Comment looked like it was already resolved.
Workflow ID: wflow_Iq0Ct3a5VFKQWQwU
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
jrmi
changed the title
jrmi
force-pushed
the
fix-tool-api-call-broken-when-no-tool-answer
branch
from
2d25236 to
ac85af3
Compare
ErikBjare
reviewed
Jan 4, 2025
jrmi
force-pushed
the
fix-tool-api-call-broken-when-no-tool-answer
branch
2 times, most recently
from
f09caaf to
871c76e
Compare
ErikBjare
reviewed
Jan 5, 2025
| messages_new | ||
| and messages_new[-1].role == "user" | ||
| and message.role == "user" | ||
| and message.call_id == messages_new[-1].call_id |
There was a problem hiding this comment.
When using the function call API, multiple calls may occur within a single answer (parallel calls). To handle this properly, ensure there is one tool response per tool call. By verifying that the call_id is distinct, we can ensure messages are not merged unless they originate from the same function call.
ErikBjare
reviewed
Jan 5, 2025
jrmi
force-pushed
the
fix-tool-api-call-broken-when-no-tool-answer
branch
from
871c76e to
420b2d9
Compare
ErikBjare
reviewed
Jan 6, 2025
jrmi
force-pushed
the
fix-tool-api-call-broken-when-no-tool-answer
branch
from
420b2d9 to
681bff4
Compare
|
Thanks again! 🥇 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This MR fixes a few bug regarding tool call in tool format:
Ctrl+cto interrupt gptmeThe underlying problems where that sometimes the
call_idwasn't set in certain situations or multiple messages where returned by thetooluse.executecall but all were the response from the tool so we needed to merge them.Important
Fixes tool API call issues by ensuring
call_idis set and merging tool responses, with updates to message handling and tests.Ctrl+c.call_idis set correctly inexecute_msg()intools/__init__.pyandToolUse.execute()intools/base.py.call_idin_merge_tool_results_with_same_call_id()inllm_openai.pyandllm_anthropic.py._handle_tools()inllm_anthropic.pyandllm_openai.pyto handle user role messages withcall_id._transform_system_messages()inllm_anthropic.pyto merge consecutive user messages with the samecall_id.test_llm_anthropic.pyandtest_llm_openai.pyto verify message conversion and tool handling withcall_id.This description was created by
for 2d25236. It will automatically update as commits are pushed.